-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Components: SlotFill: Guard property access to possibly-undefined slot #21205
Conversation
Size Change: +9 B (0%) Total Size: 856 kB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested and it does fix the crash without finding any additional issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I verified this PR fixes the issue in 5.4 👍
@diegohaz feel free to a last check on the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked that it fixes the issue! Thanks for the work @aduth! As a follow-up, I'll work on adding unit tests for this and for the other parts that may be missing coverage.
* Add failing test on slot-fill * Apply #21205 to fix the failing test
Fixes crash mentioned at #18560 (comment)
Related: #19242
This pull request seeks to update the implementation of the new SlotFill
unregisterSlot
added in #19242 to guard property access on its condition for testing an equalref
value. While based on my comment at #19242 (comment) it is not entirely clear the reason for which a duplicate call is made tounregisterSlot
when the slot is already unregistered, this avoids a situation where the absence of the slot would throw an error. The updated logic is such that if theslot
is not defined, it will fall back to to returning the previous slots valueprevSlots
, since if the slot was not defined inprevSlots
already, thenprevSlots
will continue to be the valid next state.Testing Instructions:
Verify no error occurs.
I will need some advice on implementing automated tests here. There are tests for the Slot / Fill components, but two issues:
Provider
implementation. I'm not sure if this was intended to have been removed as part of Components: Refactor SlotFill #19242. In any case, I am not sure there is complete test coverage for the new implementations added in Components: Refactor SlotFill #19242.